Skip to content

Fix: engine nits#753

Merged
mkalinin merged 7 commits intoethereum:mainfrom
julio4:patch-1
May 1, 2026
Merged

Fix: engine nits#753
mkalinin merged 7 commits intoethereum:mainfrom
julio4:patch-1

Conversation

@julio4
Copy link
Copy Markdown
Contributor

@julio4 julio4 commented Feb 17, 2026

This PR fixes a few nits I've found while reviewing specs.

@bomanaps bomanaps mentioned this pull request Feb 23, 2026
Closed
7 tasks
@MysticRyuujin
Copy link
Copy Markdown
Contributor

MysticRyuujin commented Mar 5, 2026

Good fixes overall, but two issues to address:

1. Wrong anchor for verify_cell_kzg_proof_batch

The PR changes the anchor from #verify_cell_kzg_proof_batch to #verify_cell_kzg_proof_batch_impl, but the spec text is calling the public function — _impl is the internal implementation. The anchor
should stay as #verify_cell_kzg_proof_batch.

2. Grammar in engine_getPayloadV5 description

it returns as BlobsBundleV2

Should be:

it returns BlobsBundleV2

Everything else looks correct:

  • BlobBundleV1 → BlobsBundleV1 typo fix ✅
  • SECONDS_PER_SLOT → SLOT_DURATION_MS is accurate (upstream renamed it, dev branch is 404) ✅
  • #beaconblocksbyroot-v1 and #beaconblocksbyrange-v1 anchors both exist on master ✅
  • #executionpayload anchor is valid on master ✅

@julio4
Copy link
Copy Markdown
Contributor Author

julio4 commented Mar 6, 2026

Good fixes overall, but two issues to address:

1. Wrong anchor for verify_cell_kzg_proof_batch

The PR changes the anchor from #verify_cell_kzg_proof_batch to #verify_cell_kzg_proof_batch_impl, but the spec text is calling the public function — _impl is the internal implementation. The anchor should stay as #verify_cell_kzg_proof_batch.

2. Grammar in engine_getPayloadV5 description

it returns as BlobsBundleV2

Should be:

it returns BlobsBundleV2

Good find thanks. Fixed

@MysticRyuujin MysticRyuujin mentioned this pull request Mar 17, 2026
Closed
4 tasks
@MysticRyuujin
Copy link
Copy Markdown
Contributor

MysticRyuujin commented Mar 23, 2026

@mkalinin think you could review this one?

mkalinin
mkalinin reviewed Mar 25, 2026
View reviewed changes
Comment thread src/engine/osaka.md
1. `assert len(blobsBundle.commitments) == len(blobsBundle.blobs)` and
2. `assert len(blobsBundle.proofs) == len(blobsBundle.blobs) * CELLS_PER_EXT_BLOB` and
3. `assert verify_cell_kzg_proof_batch(commitments, cell_indices, cells, blobsBundle.proofs)` (see [EIP-7594 consensus-specs](https://github.com/ethereum/consensus-specs/blob/36d80adb44c21c66379c6207a9578f9b1dcc8a2d/specs/fulu/polynomial-commitments-sampling.md#verify_cell_kzg_proof_batch))
3. `assert verify_cell_kzg_proof_batch(commitments, cell_indices, cells, blobsBundle.proofs)` (see [EIP-7594 consensus-specs](https://github.com/ethereum/consensus-specs/blob/master/specs/fulu/polynomial-commitments-sampling.md#verify_cell_kzg_proof_batch))
Copy link
Copy Markdown
Contributor

@mkalinin mkalinin Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be more stable to have a link anchored to a specific commit. wdyt?

Copy link
Copy Markdown
Contributor Author

@julio4 julio4 Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's a choice to make. My intuition is that the specs should not change drastically and master branch is considered the latest stable version.

@bomanaps
Copy link
Copy Markdown
Contributor

bomanaps commented Apr 30, 2026 •
edited
Loading

Hello @fjl @mkalinin please can we merge this?

mkalinin
mkalinin approved these changes May 1, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@mkalinin mkalinin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you!

@mkalinin mkalinin merged commit 8d6b784 into ethereum:main May 1, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mkalinin mkalinin mkalinin approved these changes

+1 more reviewer

@bomanaps bomanaps bomanaps approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@julio4 @MysticRyuujin @bomanaps @mkalinin